Skip the same-URI refresh instead of throwing on a mismatched onPost - #220
Conversation
bearsunday#214 bound CommandInterceptor to onPost, so RefreshSameCommand's automatic refresh now runs MatchQuery against every write's own URI query - including a create whose parameters do not cover onGet's required ones (a collection onPost with no id yet). MatchQuery's UnmatchedQuery propagated uncaught through CommandInterceptor's try/finally (no catch), turning a request that used to reach onPost with no interceptor at all into a hard failure. RefreshSameCommand now catches UnmatchedQuery, but only for onPost: the write already succeeded, there is no entry to refresh, and the skip is recorded as cache_error{operation: write} rather than vanishing silently. Returning instead of throwing lets CommandsProvider's next command run, so an explicit #[Refresh]/#[Purge] on the same onPost still fires. onPut/onPatch/onDelete keep throwing: those act on an entity onGet already addresses, so a required parameter missing there is a real signature mismatch, not this case - BehaviorTest::testUnMatchQuery pins that and would have caught a fix that swallowed it too. Fixes bearsunday#219. Also: CommandInterceptor's docblock said onPut/onPatch/onDelete only, stale since bearsunday#214 added onPost.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #220 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 545 549 +4
===========================================
Files 100 100
Lines 1464 1472 +8
===========================================
+ Hits 1464 1472 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Kimi K3's review of this PR found the identical bug in DonutCommandInterceptor::refreshDonutAndState(): MatchQuery called with no catch, inside invoke()'s try/finally-only block - DonutCacheModule binds this interceptor to onPost too (commandMethods()), so a #[CacheableResponse]/#[DonutCache] collection onPost with a parameter mismatch threw uncaught the same way RefreshSameCommand did. Same fix, same scoping: catch UnmatchedQuery, but only when the invoked method name starts with 'onPost' (str_starts_with, matching the AOP matcher's own startsWith semantics - RefreshSameCommand's exact-match check missed prefix-matched names like onPostItem, also flagged in review). onPut/onPatch/onDelete keep throwing on both interceptors. New fixture (MismatchedDonutWriter, #[CacheableResponse]) and DonutUnmatchedQueryRefreshTest mirror UnmatchedQueryRefreshTest for this path. Confirmed failing pre-fix via git stash on DonutCommandInterceptor.php alone (same UnmatchedQuery, same uncaught propagation through DonutCommandInterceptor.php:67).
codecov flagged DonutCommandInterceptor.php:69 (the re-throw for a non-onPost mismatch) as untested - the donut-side equivalent of BehaviorTest::testUnMatchQuery had no test. MismatchedDonutWriter gains an onPut with the same signature mismatch as its onPost; testPutStillThrowsForAGenuineMismatch pins that it still throws.
Closes #219.
#214 bound
CommandInterceptortoonPost, soRefreshSameCommand's automatic same-URI refresh now runsMatchQueryagainst every write's own URI query - including a create whose parameters do not coveronGet's required ones (a collectiononPostwith noidyet).MatchQuery'sUnmatchedQuerypropagated uncaught throughCommandInterceptor'stry/finally(nocatch), turning a request that used to reachonPostwith no interceptor at all into a hard failure - and becauseCommandsProviderordersRefreshSameCommandbeforeRefreshAnnotatedCommand, an explicit#[Refresh]/#[Purge]on the same method never ran either. #214 turned "the attribute is silently dropped" (#212) into "the attribute is silently dropped and the write 500s".The identical bug existed on the donut command path:
DonutCommandInterceptor::refreshDonutAndState()calledMatchQuerywith no catch either, inside aninvoke()whosetryisfinally-only, andDonutCacheModulebinds this interceptor toonPosttoo - so a#[CacheableResponse]/#[DonutCache]collectiononPostwith the same shape 500ed as well.Both interceptors now catch
UnmatchedQuery, scoped toonPost-prefixed methods (str_starts_with, matching the AOP matcher's ownstartsWithsemantics - the same prefix-matched names likeonPostItemthe matcher intercepts): the write already succeeded, there is no entry to refresh, and the skip is recorded ascache_error{operation: write}rather than vanishing silently.RefreshSameCommandreturning instead of throwing also letsCommandsProvider's next command run, so an explicit#[Refresh]/#[Purge]on the sameonPoststill fires.onPut/onPatch/onDeletekeep throwing on both interceptors: those act on an entityonGetalready addresses, so a required parameter missing there is a real signature mismatch, not this case -BehaviorTest::testUnMatchQuery(value-cache side) pins that, andDonutUnmatchedQueryRefreshTest::testPutStillThrowsForAGenuineMismatchpins the same contract on the donut side.Verification
MismatchedWriter(value-cache,onGet(int $id)/onPost(string $title)) andMismatchedDonutWriter(#[CacheableResponse], same shape plus anonPuttwin for the still-throws case), withtests/UnmatchedQueryRefreshTest.phpandtests/DonutUnmatchedQueryRefreshTest.php: theonPostwrite still succeeds (no throw) and is logged ascache_error; an explicit#[Purge]on the sameonPoststill runs (value-cache side);onPutwith a genuine mismatch still throwsUnmatchedQueryon both interceptors.UnmatchedQuerypropagating uncaught (reverted each source file locally viagit stash, re-ran, reverted back).BehaviorTest::testUnMatchQuery(the pre-existingonPutcase) passes unmodified.Also: both interceptors' docblocks said
onPut/onPatch/onDeleteonly, stale since #214 addedonPost.